Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

EntitlementManagement - Add Types, move catalogs and packages to 1.0 Stable #133

Merged
merged 11 commits into from
Jan 17, 2022
Merged

EntitlementManagement - Add Types, move catalogs and packages to 1.0 Stable #133

merged 11 commits into from
Jan 17, 2022

Conversation

kaovd
Copy link
Contributor

@kaovd kaovd commented Dec 11, 2021 •
edited
Loading

Bugfixes and make stable where possible parts of the EntitlementManagement scopes

ODataTypes are required to be passed on the UserSet Struct

> $ env TF_DEBUG=TRACE TF_LOG=TRACE go test -race ./msgraph -run="TestAccessPackage*"                                                                                                                                                                                                                                                                                   [±fix/identitygovernance ●]
ok      github.com/manicminer/hamilton/msgraph  143.400s

@kaovd kaovd marked this pull request as draft December 11, 2021 18:49
@kaovd kaovd marked this pull request as ready for review December 11, 2021 19:20
@kaovd
Copy link
Contributor Author

kaovd commented Dec 17, 2021

I also need to add "#microsoft.graph.question"to questions Struct, will do this later

manicminer
manicminer requested changes Dec 20, 2021
View reviewed changes
Copy link
Owner

@manicminer manicminer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @kaovd, thanks for working on this! I've made a couple of notes on compatibility between the 1.0 and beta APIs if you can consider these? There may be other divergences I have missed. If it's impractical to maintain compatibility (e.g. perhaps the beta API isn't working right) then that's fine, but for now if we can that'd be good.

msgraph/accesspackage.go Outdated Show resolved Hide resolved
msgraph/models.go Show resolved Hide resolved
msgraph/valuetypes.go Show resolved Hide resolved
odata/types.go Outdated Show resolved Hide resolved
@kaovd
Copy link
Contributor Author

kaovd commented Dec 22, 2021

Seems the test for AppRoleAssignedToClient.Assign is having issues @manicminer - do you want to try fix within the PR or just do it in as seperate fix

@kaovd kaovd mentioned this pull request Jan 6, 2022
Merged
@kaovd
Copy link
Contributor Author

kaovd commented Jan 6, 2022

Have synced with main build now passing @manicminer

@kaovd
Copy link
Contributor Author

kaovd commented Jan 10, 2022

AccessPackageResourceRequests tests are now failing for some reason likely may have changed MS side need to review

@kaovd
Copy link
Contributor Author

kaovd commented Jan 15, 2022 •
edited
Loading 

> $ go test -race ./msgraph -run="AccessPackage*" -v                                                                                                              [±fix/identitygovernance ✓]
=== RUN   TestAccessPackageClient
--- PASS: TestAccessPackageClient (8.86s)
=== RUN   TestAccessPackageAssignmentPolicyClient
--- PASS: TestAccessPackageAssignmentPolicyClient (4.77s)
=== RUN   TestAccessPackageCatalogClient
--- PASS: TestAccessPackageCatalogClient (1.63s)
=== RUN   TestAccessPackageResourceRequestClient
--- PASS: TestAccessPackageResourceRequestClient (18.34s)
=== RUN   TestAccessPackageResourceRoleScopeClient
--- PASS: TestAccessPackageResourceRoleScopeClient (17.03s)
PASS
ok      github.com/manicminer/hamilton/msgraph  50.657s

Can you give these tests a run locally @manicminer - I am not quite sure what the issues is with the CI but seem broken, they pass fine for me locally

manicminer
manicminer approved these changes Jan 17, 2022
View reviewed changes
Copy link
Owner

@manicminer manicminer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @kaovd, thanks for working on fixing this up, and sorry for the delay in reviewing. The test failure looks like a race condition stemming from the group creation - adding a sleep after line 21 in accesspackageresourcerequest_test.go seems to mitigate but we can leave that for now in lieu of a more robust way to handle propagation delays.

This LGTM! 🚀

@manicminer manicminer added this to the v0.40.0 milestone Jan 17, 2022
@manicminer manicminer added breaking change Indicates a non-backwards compatible change enhancement New feature or request package/msgraph labels Jan 17, 2022
@manicminer manicminer merged commit 96ff8d2 into manicminer:main Jan 17, 2022
manicminer added a commit that referenced this pull request Jan 17, 2022
@manicminer
Changelog for #133
eb47021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@manicminer manicminer manicminer approved these changes

Assignees
No one assigned
Labels
breaking change Indicates a non-backwards compatible change enhancement New feature or request package/msgraph
Projects
None yet
Milestone
v0.40.0
Development

Successfully merging this pull request may close these issues.
2 participants
@kaovd @manicminer